Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

update to latest light client libp2p protocol #3623

Merged
merged 11 commits into from
May 23, 2022
Merged

update to latest light client libp2p protocol #3623

merged 11 commits into from
May 23, 2022

Conversation

etan-status
Copy link
Contributor

Incorporates the latest changes to the light client sync protocol based
on Devconnect AMS feedback. Note that this breaks compatibility with the
previous prototype, due to changes to data structures and endpoints.
See ethereum/consensus-specs#2802

Due to the amount of changes, it is best to review the heavily edited
files from scratch (likewise, for the spec).

@etan-status
Copy link
Contributor Author

Tested this against the test cases in the consensus-specs repo, and also manually checked logs of local testnet to ensure the gossip is properly sent.

@etan-status etan-status force-pushed the dev/etan/lc-0.1 branch 2 times, most recently from c882132 to a7f322e Compare May 9, 2022 20:16
@github-actions
Copy link

github-actions bot commented May 10, 2022 •
edited
Loading

Unit Test Results

     12 files  ±0     850 suites  +8   58m 20s ⏱️ + 4m 40s
1 693 tests ±0  1 645 ✔️ ±0    48 💤 ±0  0 ❌ ±0 
9 861 runs  +8  9 741 ✔️ ±0  120 💤 +8  0 ❌ ±0 

Results for commit 4395b19. ± Comparison against base commit 1e6b32b.

♻️ This comment has been updated with latest results.

@etan-status
update to latest light client libp2p protocol
917b98f 
Incorporates the latest changes to the light client sync protocol based
on Devconnect AMS feedback. Note that this breaks compatibility with the
previous prototype, due to changes to data structures and endpoints.
See ethereum/consensus-specs#2802

Due to the amount of changes, it is best to review the heavily edited
files from scratch (likewise, for the spec).
@etan-status
reduce stack size (Linux arm64)
235a916
@etan-status
avoid randomness outliers in light client tests
d82b9c4
@etan-status
Copy link
Contributor Author

Ran CI tests 5 times, flakiness seems to be gone now. There was some randomness involved, in the way how we simulated sync committee signatures, where sometimes it would generate too many and would actually reach supermajority, breaking the test case assumption. This is now fixed by putting a cap on randomization to not exceed the supplied target participation percentage.

@tersec
Copy link
Contributor

tersec commented May 12, 2022

Linux i386 Nim 1.2:

[Suite] Light client [Preset: mainnet]
  [OK] Pre-Altair
    /home/runner/work/nimbus-eth2/nimbus-eth2/tests/test_light_client.nim(169, 29): Check failed: store.finalized_header == finalityUpdate.get.finalized_header
    store.finalized_header was (slot: 8192, proposer_index: 1, parent_root: fdedfe9932eb02cfb9380be5c86edfa03e54067726c084e6b29a440b16249233, state_root: f546a3f3e654a863614395ff7effdaf74850ddb3d23c6aa77ae54b7557efe74a, body_root: 2e0c9e75376f5dd11b4ba65c3688f265c84f7898fbc41908efc456856cdbfbff)
    finalityUpdate.get.finalized_header was (slot: 16384, proposer_index: 21, parent_root: 5d0d71a63d55c2ec04f148a2a57a9f9486b1906597b06fb01f9b0078f22bd869, state_root: 2d456114eb16f369a93b973d46ba2ff605f55dd45414afdc3c4f5375b98c398e, body_root: f8cdeb7bbb0e6061b17a21e7683ff124b13ab115265399c7ebe2c83a338b5376)
  [FAILED] Light client sync
  [OK] Init from checkpoint

@tersec
Copy link
Contributor

tersec commented May 13, 2022

https://ci.status.im/blue/organizations/jenkins/nimbus%2Fnimbus-eth2%2Fmultibranch/detail/PR-3623/19/pipeline/87 shows the same thing, even after 9bcc2b3:

[Suite] Light client [Preset: mainnet]
  [OK] Pre-Altair
    /Users/jenkins/workspace/_nimbus-eth2_multibranch_PR-3623/macos_arm64/tests/test_light_client.nim(169, 29): Check failed: store.finalized_header == finalityUpdate.get.finalized_header
    store.finalized_header was (slot: 8192, proposer_index: 1, parent_root: fbbf0fc9bd94b04c596782cb49853466858a607c7cf3d67a0dc92bc34d8047c4, state_root: f9a3981b961bf781b820603304da3554f7c1fd79d07c1a432b5f0c02c60b25cf, body_root: 99731a01c1c648a16454e1d6dc79335c1804f2f647ec394a99f98f5088f29895)
    finalityUpdate.get.finalized_header was (slot: 16384, proposer_index: 21, parent_root: 5bbfc1240f2524cf9c817541ac3b4256065050fed506c0b9ff8f0a266911c288, state_root: c64332da8ad60c7868de5dc26c2507ec560b205853829779be72b25a4faab54e, body_root: 792ac60a443e2052cc6f52067d480d273f2ed493bbf0661a9e3935a68dd09b7e)
  [FAILED] Light client sync
  [OK] Init from checkpoint

arnetheduck
arnetheduck reviewed May 17, 2022
View reviewed changes
beacon_chain/consensus_object_pools/block_pools_types_light_client.nim Outdated
## Latest slot for which updates were broadcasted on libp2p gossip.
## Tracks `update.signature_slot`.

broadcastGossipFut*: Future[void]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this belongs in BeaconNode (which is where validator_duties keeps things for its own needs)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved it into a separate pool type, as it is also used in gossip_validation (minus the Future). I think it's a nice compromise between having it in the block_pools_types_light_client where it clearly does not belong, and fragmenting these few vars across too many files.

@etan-status
Copy link
Contributor Author

Manually validated against light client poc.

@etan-status etan-status merged commit c808f17 into unstable May 23, 2022
@etan-status etan-status deleted the dev/etan/lc-0.1 branch May 23, 2022 12:02
@etan-status etan-status mentioned this pull request May 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tersec tersec tersec approved these changes

@arnetheduck arnetheduck arnetheduck left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@etan-status @tersec @arnetheduck